-
Notifications
You must be signed in to change notification settings - Fork 400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mezoification: Add base state and UI elements #3785
base: main
Are you sure you want to change the base?
Conversation
9d1947a
to
4425ed7
Compare
8fdacc1
to
6143867
Compare
4425ed7
to
12eda39
Compare
This will control visibility and behaviour of banners and other UI elements
This sets a recurrent alarm to hit the API and check claim state unless the wallet is not eligible or the campaign has already been finished. If eligible, users receive a notification based on their claim completion.
Transactions must have been completed and target the borrower contract on the mezo network.
This is used by posthog to distinguish users
- onDismiss is no longer passed as an options to the extension API - Fixed a race condition in NotificationService state initialization - Fixed the notifications settings toggler As MV3 requires permissions to be requested during a user action, the prompt for user permission no longer works if triggered through a redux thunk that executed in the background.
12eda39
to
6422370
Compare
Sets a visibility assertion to confirm element exists before synchronously asserting value
Exclusively used for newly added builtin networks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few initial thoughts, haven't made it all the way through yet.
@@ -47,6 +54,13 @@ export type UIState = { | |||
routeHistoryEntries?: Partial<Location>[] | |||
slippageTolerance: number | |||
accountSignerSettings: AccountSignerSettings[] | |||
activeCampaigns: { | |||
"mezo-claim"?: { | |||
dateFrom: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing if we make this Date
it won't (de)serialize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it will remain a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should give our serializer/deserializer the ability to deal with dates tbh (not as a blocker though).
@@ -134,6 +137,15 @@ export default class AnalyticsService extends BaseService<Events> { | |||
await super.internalStopService() | |||
} | |||
|
|||
get analyticsUUID() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From below it looks like we can lose this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're referring to the error below, that's just to block access until service has initialized. This could happen during tests or if there's an attempt to get this data from within another service's initialization without awaiting service.started()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant that it doesn't look like we're accessing this as a property anywhere—which is good, as leaking the UUID feels like leaking the private state of the analytics service and leaking the implementation detail of using PostHog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I'm not following, we need this id to get the wallet's campaign state 🤔
) { | ||
return | ||
} | ||
|
||
const shownItems = new Set( | ||
await this.preferenceService.getShownDismissableItems(), | ||
) | ||
|
||
// const uuid = this.analyticsService.analyticsUUID | ||
const hasSeenEligibilityPush = shownItems.has("mezo-eligible-notification") | ||
const hasSeenBorrowPush = shownItems.has("mezo-borrow-notification") | ||
const hasSeenNFTNotification = shownItems.has("mezo-nft-notification") | ||
|
||
// fetch with uuid | ||
const campaignData = { | ||
dateFrom: "2025-02-21", | ||
dateTo: "2025-03-28", | ||
state: "eligible" as MezoClaimStatus, | ||
} | ||
|
||
const isActiveCampaign = dayjs().isBetween( | ||
campaignData.dateFrom, | ||
campaignData.dateTo, | ||
"day", | ||
"[]", | ||
) | ||
|
||
if ( | ||
isActiveCampaign && | ||
campaignData.state === "eligible" && | ||
!hasSeenEligibilityPush | ||
) { | ||
this.notificationsService.notify({ | ||
options: { | ||
title: | ||
"Enjoy 20,000 sats on Mezo testnet. Try borrow for an exclusive Mezo NFT!", | ||
message: "Login to Mezo to claim", | ||
onDismiss: () => | ||
this.preferenceService.markDismissableItemAsShown( | ||
"mezo-eligible-notification", | ||
), | ||
}, | ||
callback: () => { | ||
browser.tabs.create({ url: "https://mezo.org/matsnet" }) | ||
this.preferenceService.markDismissableItemAsShown( | ||
"mezo-eligible-notification", | ||
) | ||
}, | ||
}) | ||
} | ||
|
||
if ( | ||
isActiveCampaign && | ||
campaignData.state === "claimed-sats" && | ||
!hasSeenBorrowPush | ||
) { | ||
this.notificationsService.notify({ | ||
options: { | ||
title: "Borrow mUSD with testnet sats for an exclusive Mezo NFT!", | ||
message: "Click to borrow mUSD ", | ||
onDismiss: () => | ||
this.preferenceService.markDismissableItemAsShown( | ||
"mezo-borrow-notification", | ||
), | ||
}, | ||
callback: () => { | ||
browser.tabs.create({ url: "https://mezo.org/matsnet/borrow" }) | ||
this.preferenceService.markDismissableItemAsShown( | ||
"mezo-borrow-notification", | ||
) | ||
}, | ||
}) | ||
} | ||
|
||
if ( | ||
isActiveCampaign && | ||
campaignData.state === "borrowed" && | ||
!hasSeenNFTNotification | ||
) { | ||
this.notificationsService.notify({ | ||
options: { | ||
title: | ||
"Spend testnet mUSD in the Mezo store for an exclusive Mezo NFT!", | ||
message: "Click to visit the Mezo Store", | ||
onDismiss: () => | ||
this.preferenceService.markDismissableItemAsShown( | ||
"mezo-borrow-notification", | ||
), | ||
}, | ||
callback: () => { | ||
browser.tabs.create({ url: "https://mezo.org/matsnet/borrow" }) | ||
this.preferenceService.markDismissableItemAsShown( | ||
"mezo-borrow-notification", | ||
) | ||
}, | ||
}) | ||
} | ||
|
||
this.store.dispatch(updateCampaignState(["mezo-claim", campaignData])) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for our quick implementation here, but we really need to move this either into redux itself or out to a service—the ReduxService should be doing the minimum possible to coordinate between the two. More like what the island service hookup at https://github.com/tahowallet/extension/blob/main/background/services/redux/index.ts#L1567-L1569 does.
Need to do some deeper arch work to figure that out before we can clean up since these are more complex interactions though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for giving this a look, wasn't confident enough to put this into it's own service without receiving some feedback on the approach first 🕵️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a few ways in my head we should factor it out:
- Mezo service that handles Mezo-related things including these kinds of campaigns.
- Notification service is directly responsible for stuff like this (not how we originally envisioned it, but may make sense).
- Campaigns service to handle these kinds of campaigns, Mezo service would handle more network-level/API stuff like mats, Passport, etc.
Moves test network visibility user setting to preference service and sets force toggles it to display on all new and existing installs
|
||
type TopMenuProtocolListProps = { | ||
onProtocolChange: (network: EVMNetwork) => void | ||
} | ||
|
||
/** | ||
* Places Ethereum and Mezo network above other networks | ||
* Places Mezo network above other networks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ngl I would love to kill this sort function. Its main purpose is to boost matsnet in the list if people have previously added it, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that's pretty much all it does. Currently testnets are hardcoded so we could remove it and use the hardcoded order but that seems a bit dirty 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work even if the user had previously added it as a custom network? If so, then let's rely on the hardcoded order for now.
isSelected={sameNetwork(currentNetwork, network)} | ||
key={network.name} | ||
network={network} | ||
info={testNetworkInfo[network.chainID]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is descriptively named info
lol—is this the description that lives under the network name? If so maybe let's rename to description
(not a blocker).
"function openTrove(uint256 _maxFeePercentage, uint256 debtAmount, uint256 _assetAmount, address _upperHint, address _lowerHint)", | ||
]) | ||
|
||
// eslint-disable-next-line import/prefer-default-export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint disable is best accompanied by an explanation.
In this case my thought is that we probably move this into a Mezo or Mezo campaign service, which is where we deal with the Redux service's complexity as well (after we ship this, I mean). Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, this is only used for the campaign. I initially put this under /lib
because I thought this would be also used by the enrichment service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. I think once we're enriching we'll probably export more anyway—and I wonder if the better approach here is actually making this file an ABI export file (background/lib/mezo/musd-trovemanager-abi.ts
or so) and then we implement checkIsBorrowingTx
directly where we need it and let enrichment do its own things with the ABI.
Anyway, none of that is blocking.
This PR moves the testnet visibility setting to the preference service, and sets a migration to enable it on all existing and new installs. ## Testing - [ ] On a new install, check testnets shows up by default on network switcher - [ ] On existing install, after update testnets should show up Latest build: [extension-builds-3793](https://github.com/tahowallet/extension/suites/35160394180/artifacts/2686423977) (as of Tue, 04 Mar 2025 05:05:23 GMT).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round, still cookin'.
@@ -222,6 +237,22 @@ const uiSlice = createSlice({ | |||
...state, | |||
settings: { ...state.settings, autoLockInterval: payload }, | |||
}), | |||
updateCampaignState: <T extends keyof UIState["activeCampaigns"]>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we start pulling typeof
s like this it's usually a sign to me that we should name the type, fwiw.
@@ -47,6 +54,13 @@ export type UIState = { | |||
routeHistoryEntries?: Partial<Location>[] | |||
slippageTolerance: number | |||
accountSignerSettings: AccountSignerSettings[] | |||
activeCampaigns: { | |||
"mezo-claim"?: { | |||
dateFrom: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should give our serializer/deserializer the ability to deal with dates tbh (not as a blocker though).
@@ -134,6 +137,15 @@ export default class AnalyticsService extends BaseService<Events> { | |||
await super.internalStopService() | |||
} | |||
|
|||
get analyticsUUID() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant that it doesn't look like we're accessing this as a property anywhere—which is good, as leaking the UUID feels like leaking the private state of the analytics service and leaking the implementation detail of using PostHog.
@@ -277,18 +276,10 @@ export default class PreferenceService extends BaseService<Events> { | |||
} | |||
|
|||
async setShouldShowNotifications(shouldShowNotifications: boolean) { | |||
if (shouldShowNotifications) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to still make it so that this can only set the db state if the permission is actually granted, rather than having it be a blind state updater. i.e., it would be good if we could guard against setShouldShowNotifications(true)
successfully updating the db if in fact the user denied the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm we can verify if permission has indeed been granted and guard that I think
shouldShowNotifications, | ||
) | ||
this.store.dispatch(toggleNotifications(isPermissionGranted)) | ||
await this.preferenceService.setShouldShowNotifications( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently hooked up to the Show Subscape notifications
settings toggle. Two problems I think:
- By removing the
permissions.request
from the service, we ensure that turning that toggle on will not work, as there's no permission request prompt on the click path. - We should probably rename that setting to
Show Mezo notifications
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, I had to move permissions.request to the UI because in mv3 permission requests have to trigger in response to a user interaction and that detail seems to get lost on the way to the service worker 😕
@@ -550,7 +550,8 @@ | |||
"l2": "L2 scaling solution", | |||
"compatibleChain": "EVM-compatible blockchain", | |||
"avalanche": "Mainnet C-Chain", | |||
"connected": "Connected" | |||
"connected": "Connected", | |||
"featuredNetwork": "new" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it featured or new? 😉
Yeah should be a full dismissal. |
Adds base state, notification handlers and banner elements for upcoming campaign...
Banners and notifications should change based on the state of the campaign.
To Test
SUPPORT_MEZO_NETWORK=true
open the wallet, after a minute, a banner should appearLatest build: extension-builds-3785 (as of Thu, 06 Mar 2025 12:56:36 GMT).